select.lua: select from the watch history with g-h #15655

guidocella · 2025-01-05T22:22:56Z

Implement saving watched paths and selecting them.

--osd-playlist-entry determines whether titles and/or filenames are shown. But unlike in show-text ${playlist} and select-playlist, "file" and "both" print full paths because history is much more likely to have files from completely different directories, so showing the directory conveys where files are located. This is particularly helpful for filenames like 1.jpg.

The last entry in the selector deletes the history file, as requested by Samillion.

The history could be formatted as CSV, but this requires escaping the separator in the fields and doesn't work with paths and titles with newlines, or as JSON, but it is inefficient to reread and rewrite the whole history on each new file, and doing so overwrites the history with an empty file when writing without disk space left. I went with an hybrid of one JSON array per line to get the best of both worlds. And I discovered afterwards that this was an existing thing called NDJSON or JSONL. Since there are these 2 competing standards it is not clear if the file extension should be ndjson or jsonl, so I just used txt.

watch_history_path is awkwardly documented along with the key binding because I don't think it's worth adding a select.lua section to the manual just for this. I will add it and move it there if I add more script-opts in the future.

github-actions · 2025-01-05T22:45:02Z

Download the artifacts for this pull request:

Windows

macOS

DOCS/man/mpv.rst

player/lua/select.lua

mrfragger · 2025-01-06T04:56:23Z

This works well. Comparing it to memo.lua which is a history script. It appends at end of file. When items hit around 400 history entries or so it won't open in one or two seconds. Sometimes it takes 5 seconds or more. So I trim the history file just keeping the last 333 entries.

# run "bin/bash" "-c" "cd ~/.config/mpv; cat memo-history.log | tail -n 333 > memo.log ;rm memo-history.log; mv memo.log memo-history.log";show-text "Trimmed ~/.config/mpv/memo-history.log to 333 lines to speed up opening History memo" #! [History Chapters Bookmarks] > Trim History log if slow to open

It doesn't show duplicates which is fine. However, a suggestion I have if it doesn't create a problem with long text that won't fit on the osd that is. Not sure if it's wrapped or what. Is to add the current chapter within the file appending it after a # symbol.

In the bookmarker-menu.lua I modified it to save bookmarks with the current chapter. This will help differentiate rather than just timestamps or time position to indicate where one is within the file.

function makeBookmark(bname)
    local chaptername = mp.get_property("chapter-metadata/title") or ""
    if mp.get_property("path") ~= nil then
        if bname == nil then bname = mp.get_property("media-title") .. " #" .. chaptername .. " @ %t" end
        local bookmark = {
            name = parseName(bname),
            pos = mp.get_property_number("time-pos"),
            path = parsePath(mp.get_property("path")),
            version = 2
        }
        return bookmark
    else
        return nil
    end
end

input.conf

g-h script-binding select/select-watch-history #! [Utilities] > [select.lua] > Open History
g-w script-binding select/select-watch-later #! [Utilities] > [select.lua] > Select Watch Later

mpv.conf

save-watch-history=yes
write-filename-in-watch-later-config=yes

guidocella · 2025-01-06T07:49:58Z

This has a clear history entry and is usable with tens of thousands of lines, in fact I was testing the performance of searching in 150k lines yesterday and it was still usable if we switch to the C version of fzy, though ~~opening it is slow~~. It opens pretty fast now, 0.2 seconds for 100k lines. Apparently prepending to tables is very slow in Lua.

This doesn't trim duplicates but it doesn't really have duplicates as files are logged with different timestamps every time which we can be useful to check.

Text longer than the OSD is cropped. This appends to history when files are opened so there is no chapter or time position information at all. We have watch later files for resuming from the last position, so I assume you were talking about the watch later selection? That also doesn't show any time position, let alone chapters. I don't know if showing the time position before resuming is needed, I have never felt the need for that as it automatically resumes from that position. If you mean to use it for bookmarks of scenes watch later is not suitable for that since config files are deleted on resume, EDL is.

mrfragger · 2025-01-06T09:09:53Z

sounds great performance wise. "Clear history" sounds better than "Clear the history"

Oh wasn't aware time it appends upon open so yeah that's a no-go like you say. So watch-later probably wouldn't work since path is there already and text would be too long.

Forgot also on memo.lua I add chapters to the history entries

    if not mark_hidden then
        local chaptername = mp.get_property("chapter-metadata/title") or ""
        local playlist_pos = mp.get_property_number("playlist-pos") or -1
        local title = playlist_pos > -1 and mp.get_property("playlist/"..playlist_pos.."/title") or ""
        local title_length = #title + #chaptername + 2
        local timestamp = os.time()

        entry = timestamp .. "," .. (title_length > 0 and title_length or "") .. "," .. title .. " #" .. chaptername .. "," .. full_path
    elseif last_state then
        last_state.hidden_files[full_path] = last_state.current_page * 10000 + item_index
    end

kasper93 · 2025-01-06T09:20:14Z

though opening it is slow.

If opening is slow, we should consider not using json. It brings no value. Simple separated values in each line will work the same and avoid parsing json.

This doesn't trim duplicates but it doesn't really have duplicates as files are logged with different timestamps every time which we can be useful to check.

I'd prefer for it to be de-duplicated in select, else searching for a file, with list this file with multiple dates, which makes searching history more troublesome.

avih · 2025-01-06T09:39:32Z

I was testing the performance of searching in 150k lines

I don't have a real stake in this, but since it already came up before, IMO the range of input sizes should be discussed, and agreed upon, because this can affect the solution.

Personally I don't care about 150K lines of anything. I don't use playlists with more than 20 items, I don't have thousands of tracks I need to choose from, etc. I'm using my own console with my own history implementation (deduplicatd, recent-sorted), and I know that it just doesn't keep growing beyond orders of magnitude less than 150K, despite me using it very frequently for many years now, without cleaning up the history.

But maybe there could be cases of many many thousands of entries, like playlists, etc, that people agree should be handled well, and in that case the target sizes should be agreed.

If there's an agreement that such sizes are required to be handled well, then IMO more appropriate solutions should also be considered, like sqlite, or some performant key-value store etc.

Maybe specifically 150K can still be handled carefully without external tools/libraries, also on the RPI, but at some stage, if the size being mentioned keeps growing - as it seems to be happening all the time, it can becomes too big to keep handling it internally in some naive fashion IMO.

guidocella · 2025-01-06T09:43:36Z

How can loading 150k lines not be slow JSON or not? CSV will not work or is not simple when paths or titles contain newlines, and a new parser may be slower if not written in C like parse_json. e.g. LuaJIT fzy is over 10 times slower than the C version. It opens perfectly fast with sane history sizes.

kasper93 · 2025-01-06T09:55:17Z

How can loading 150k lines not be slow JSON or not?

Well, there are databases that processes billions of rows fast. It's all matter of perspective.

You brought up that it is slow. So I'm offering a different view on this. Whether we want to keep it slow is another question.

not written in C

There is nothing wrong with C. Whole mpv is written in C. watch-later is also C and I believe it correctly escapes filenames, no?

EDIT:

Also loading a full file on every search is slow, which could be easily avoided. Again the question is how much slowness is acceptable.

guidocella · 2025-01-06T10:03:20Z

Databases can search through billions of rows quickly with indexes but if you actually retrieve all of them at once to use them somewhere else that is slow.

watch later indeed escapes incorrectly since it converts newlines to _, such files don't work in the watch later selector. There was also an issue about cookie options with newlines not working in watch later files.

Added deduplication.

kasper93 · 2025-01-06T10:04:55Z

watch later indeed escapes incorrectly since it converts newlines to _, such files don't work in the watch later selector. There was also an issue about cookie options with newlines not working in watch later files.

Should this be fixed?

player/lua/select.lua

guidocella · 2025-01-06T10:28:48Z

Should this be fixed?

It seems the author already had a fix in #13016, I didn't see that before. Though paths in comments would have to be fixed separately from options, select.lua doesn't have access to the config file parser. At least with JSON we get it working for free.

sfan5 · 2025-01-06T10:39:42Z

DOCS/man/options.rst

+
+``--save-watch-history``
+    Whether to save which files are played (default: no). These can be then
+    selected with the default ``g-h`` key binding.


documenting it like this might make people think it's an option built into the mpv core, so it should say that it belongs to select.lua (or at least that it requires Lua)

Well it could have been kept as a script-opt if that's an issue.

Is it an issue? It's in implementation detail how it's implemented imho.

Well I think it would be nice to know what exactly you lose when you disable Lua, but it's not of high importance.

This should remain a script-opt. No other core options do nothing, like this one.

Same as many other options that do nothing if mpv is compiled without some library dependency.

See #14624: the direction is that core options should either warn or don't exist at all when compiled without dependency. And there will be even more complexity when dealing with this history feature which can be unloaded at runtime.

Do we want to add a 20 line script just to append to the history? Since the writing code is 20 lines vs 90 lines for the selection code, the selection code needs to know exactly how the writing code above formats the JSON, and they both needed the same watch_history_path script-opt, I thought they might as well be together. And if not using global options it would have been very confusing if users have to configure script-opts related to saving the history in history.lua and script-opts related to reading and formatting the history in select.lua, and that 1 of the 2 scripts has to call read_options with the name of the other script to get shared script-opts like watch_history_path. Not sane.

select.lua doesn't really handle selection, it gathers and formats data to be selected in console. I don't see how gathering and formatting history is different from any of its other selectors.

I don't understand what not overriding OSD styles has to do with options vs script-opts for these history ones. I made scripts respect existing OSD options when feasible to not make users configure the same thing multiple times. Actually this does exactly the same thing with --osd-playlist-entry. But IMO it makes sense that options that only have effect within a script are its script-opts, it makes it clear what script they affect, what script you need to edit if you want to alter the behavior, and that you lose that functionality by disabling Lua. It also avoids bloating global options if this gains several script-opts in the future, imagine if every OSC script-opt was a global option. Maybe there is value in having easier to type shortcuts like we already have in --ytdl-format and --ytdl-raw-options but dunno if that's worth losing clarity and consistency. At that point if we want to make it more usable a better way would be some GUI button that appends the option to enable history in mpv.conf.

Maybe only the writing part can be converted to C since that's small, after mp_delete_watch_later_conf(). That provides a reason to use global options.

We really need to put an end to this ongoing debate about Lua vs. C implementation.

This issue came up in the other PR as well. Initially, it was implemented in C, then it was requested to be reimplemented in Lua. Afterward, a major concern arose: "What if Lua isn't enabled?"... can we not troll ourselves again?

We need to make a clear decision: either we allow "core" functionalities to be implemented in Lua, or we don't. We can't keep standing in the middle of the road, indecisive, every time this comes up.

In my personal opinion which may not reflect opinion of others. I think implementing features like history in Lua is fine and many of mpv features could be implemented as such. It is also valid to implement it in C, in this cases, there is not really a difference, as the code to save/load watch-history is trivial in both cases (it's calling our json parser anyway). I'm fine with having it in Lua. But at the same time, I think it should be considered "core" option, meaning options should be top-level, not script-opt, as the fact it is implemented in Lua shouldn't dictate the user interface to configure such feature. And if this is a problem, we should stop implementing features in Lua.

I'm fine with the current state of this PR. I don't see technical issue with the current implementation. It's small, works. But if others don't agree on this, we will need to think of other implementation. But please make it clear that we don't want to use Lua, so we don't make this mistake again in the future.

The only concern about Lua implementation is that, each new script spawns new mpv_client with own event loop, which may not be a big deal, but the constant overhead is there. I agree though that putting everything in select.lua is bit out of place. But in the same time, spawning clients for 30 lines scripts is little bit meh.

EDIT: Also lua code will be bigger than compiled C code in final binary. So there is that.

Maybe only the writing part can be converted to C since that's small

Yeah, maybe. I think in this case small save function in C will resolve all concerns about options and need for separate history.lua and select.lua will have only selector, same as for watch-later. The save code in C will be less than it is in Lua actually...

player/lua/select.lua

verygoodlee · 2025-01-06T17:16:29Z

history file may expose privacy-sensitive information, request to add a blacklist option to prevent some files from being stored in the history file, the syntax can be consistent with ytdl_hook-exclude .

player/loadfile.c

player/lua/select.lua

kasper93 · 2025-01-26T05:28:32Z

player/loadfile.c

+    void *ctx = talloc_new(NULL);
+    char *history_path = mp_get_user_path(ctx, mpctx->global,
+                                          mpctx->opts->watch_history_path);
+    FILE *history_file = fopen(history_path, "ab");


I was also thinking if we should use stream.c for this writing, instead of NIHing. Though stream.c doesn't support append currently, so it would need to be extended. And maybe for single write it is not that needed.

player/loadfile.c

misc/json.h

player/loadfile.c

player/misc.c

This will be used to write the title in the watch history.

This will be used to write json with a bstr in the next commit. Co-authored-by: Kacper Michajłow <[email protected]>

The history could be formatted as CSV, but this requires escaping the separator in the fields and doesn't work with paths and titles with newlines. Or as JSON, but it is inefficient to reread and rewrite the whole history on each new file, and doing so overwrites the history with an empty file when writing without disk space left. So this uses a hybrid of one JSON object per line to get the best of both worlds. This is called NDJSON or JSONL. Co-authored-by: Kacper Michajłow <[email protected]>

Implement selection of the entries in the watch history. The last entry in the selector deletes the history file.

mrfragger · 2025-01-28T21:46:59Z

(date) filename.ext (/user/path/filename.ext)

Think lots will complain of the filename.ext being put twice as it's redundant. If the reasoning is it's computationally expensive to strip off filename.ext from path then I suppose that's understandable. Short names no biggie as it's not an issue but usually those with long filenames and long paths doesn't make sense.

If I wish to know where I file is located I can either open file manager where file is located
F5 script-binding uosc/show-in-directory #! [Utilities] > Show File in Directory

or can show the path of filename in the msg-osd with copystuff.lua

-- Copy Full Filename Path
local function copyFullPath()
if platform == WINDOWS then
full_path = string.format("%s\%s", mp.get_property_osd("working-directory"), mp.get_property_osd("path"))
else
full_path = string.format("%s/%s", mp.get_property_osd("working-directory"), mp.get_property_osd("path"))
end

if set_clipboard(full_path) then
    mp.osd_message(string.format("Full Filename Path Copied to Clipboard: %s", full_path))
else
    mp.osd_message("Failed to copy full filename path to clipboard")
end

end

input.conf
s-i script-binding copyFullPath #! [Utilities] > [Copy to Clipboard] > Full Path

guidocella · 2025-01-28T21:50:38Z

The filename is no longer printed twice.

mrfragger · 2025-01-30T07:07:21Z

oh ok..I just downloaded lastest build from latest pull request but still shows
(date) filename.ext (/user/path/filename.ext)

instead of
(date) filename.ext (/user/path)

kasper93 reviewed Jan 5, 2025

View reviewed changes

guidocella force-pushed the history branch 2 times, most recently from fd03b7c to d80ee92 Compare January 5, 2025 23:18

guidocella force-pushed the history branch 2 times, most recently from 423c3ba to 30e0c38 Compare January 6, 2025 09:07

guidocella force-pushed the history branch from 30e0c38 to 95e6722 Compare January 6, 2025 09:14

guidocella force-pushed the history branch from 95e6722 to bd3a159 Compare January 6, 2025 09:56

kasper93 reviewed Jan 6, 2025

View reviewed changes

player/lua/select.lua Outdated Show resolved Hide resolved

sfan5 reviewed Jan 6, 2025

View reviewed changes

guidocella force-pushed the history branch 3 times, most recently from 02ef821 to 6afdfe2 Compare January 6, 2025 13:27

kasper93 reviewed Jan 6, 2025

View reviewed changes

player/lua/select.lua Show resolved Hide resolved

kasper93 reviewed Jan 6, 2025

View reviewed changes

player/lua/select.lua Outdated Show resolved Hide resolved

guidocella force-pushed the history branch 2 times, most recently from 382af52 to d0e0679 Compare January 6, 2025 14:56

kasper93 reviewed Jan 6, 2025

View reviewed changes

player/lua/select.lua Show resolved Hide resolved

player/lua/select.lua Show resolved Hide resolved

guidocella force-pushed the history branch from d0e0679 to cc50c4b Compare January 6, 2025 16:10

guidocella force-pushed the history branch 3 times, most recently from 9f8f9b1 to 35737cb Compare January 10, 2025 09:36

guidocella mentioned this pull request Jan 20, 2025

DOCS/man/select: add this file #15713

Merged

kasper93 reviewed Jan 25, 2025

View reviewed changes

player/loadfile.c Outdated Show resolved Hide resolved

guidocella force-pushed the history branch from 35737cb to 637981e Compare January 25, 2025 19:25